ZOOKEEPER-4712: Follower.shutdown() and Observer.shutdown() do not correctly shutdown the syncProcessor, which may lead to data inconsistency - #2032
Conversation
…rrectly shutdown the syncProcessor, which may lead to data inconsistency
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
| if (shutdownZKServer(fullyShutDown)) { | ||
| try { | ||
| if (syncProcessor != null) { | ||
| syncProcessor.shutdown(); |
There was a problem hiding this comment.
THREAD_SAFETY_VIOLATION: Unprotected write. Non-private method LearnerZooKeeperServer.shutdown(...) indirectly writes to field this.syncProcessor.lastFlushTime outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
ℹ️ Expand to see all @sonatype-lift commands
You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.
| Command | Usage |
|---|---|
@sonatype-lift ignore |
Leave out the above finding from this PR |
@sonatype-lift ignoreall |
Leave out all the existing findings from this PR |
@sonatype-lift exclude <file|issue|path|tool> |
Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file |
Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.
tisonkun
left a comment
tisonkun
left a comment
There was a problem hiding this comment.
Can you add a test case to simulate the bug you spot? I can understand it while a test case is expressive and prevent regressions.
| * @param fullyShutDown true if another server using the same database will not replace this one in the same process | ||
| * @return true if the server is successfully shutdown, false if the server cannot be shutdown. | ||
| */ | ||
| public synchronized boolean shutdownZKServer(boolean fullyShutDown) { |
There was a problem hiding this comment.
Will these two new methods be protected? I don't think they are intended to be called by app users.
There was a problem hiding this comment.
Yeah I think protected will be enough for these two methods. I will correct it later.
Sure I will try. Note that the bug is subtle and needs to be exposed with careful timing of multi-thread events. I will try to build a test case for the bug triggering. |
6191038 to
c1c4d87
Compare
…rrectly shutdown the syncProcessor, which may lead to data inconsistency
|
Superceded by #2154. Thank you for your contribution! @AlphaCanisMajoris |
3 participants
See ZOOKEEPER-4712 for more details.
This fix ensures that Follower.shutdown() and Observer.shutdown() will invoke syncProcessor.shutdown() correctly.
Besides, it ensures that syncProcessor.shutdown() will happen before zkDb.fastForwardDataBase(), which avoids possible data inconsistency.